fix: preserve ';', '?', '#' in object keys during resolve()#86
Open
TomNicholas wants to merge 1 commit into
Open
fix: preserve ';', '?', '#' in object keys during resolve()#86TomNicholas wants to merge 1 commit into
TomNicholas wants to merge 1 commit into
Conversation
ObjectStoreRegistry.resolve() derived the object key via urlparse(url).path,
which silently drops anything urlparse treats as URL params (';'), query ('?')
or fragment ('#'). These characters are valid in S3/GCS/Azure object keys
(e.g. 'DC_DCAM#1_CAM3.tif' in the Cell Painting Gallery), so the key was
truncated and lookups failed with a spurious FileNotFound.
Reattach the params/query/fragment to reconstruct the full key. For HTTP-backed
stores this also preserves query strings (presigned URLs, Azure SAS tokens),
which were previously dropped.
kylebarron
approved these changes
Jul 20, 2026
maxrjones
requested changes
Jul 20, 2026
maxrjones
left a comment
maxrjones
left a comment
Member
There was a problem hiding this comment.
Thanks for your work on this @TomNicholas. I don't think this is the right fix though, for a few reasons:
- It doesn't fully solve motivation 1 from #85, since a key ending in a bare
;,?or#parses to an empty component, so theif parsed.queryguard skips it and the character is still lost. - It breaks prefix matching when a query lands at a registration boundary (e.g.,
https://example.com/data?token=xwill match on the segmentdata?token=xcausing a regression. - It widens the credential leakage surface since SAS tokens and presigned signatures now ride along with the path into tracing and caching wrappers.
I think the right fix is scheme-based dispatch. The object store schemas should have the suffix after the netloc as an opaque key rather than using urlparse at all.
I'd also split out motivation 2 from #85. I think it should be tackled as part of the AiohttpStore overall in #65, since providing a way to route auth is distinct from path handling.
Are you interested in splitting up this PR and adjusting the solution to motivation 1 as requested above?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #85.
Problem
ObjectStoreRegistry.resolve()derived the object key viaurlparse(url).path, which silently drops URL params (;), query (?), and fragment (#). So any URL containing those characters had its key truncated.Two real motivations (details in #85):
#/?/;— valid in S3/GCS/Azure keys. E.g. the Cell Painting Gallery has files namedDC_DCAM#1_CAM3.tif;resolve()returned.../DC_DCAM, causing a spuriousFileNotFound.?...strips the auth, so every read 403s. Preserving the query lets a reader access a private bucket via self-authenticating URLs with no local credentials.Change
registry.py: reattach params/query/fragment to reconstruct the full key.Behavior
s3://bucket/path/DC_DCAM#1_CAM3.tifpath/DC_DCAM❌path/DC_DCAM#1_CAM3.tif✅s3://bucket/path/obj?v=1.tifpath/obj❌path/obj?v=1.tif✅https://host/file.nc?token=abcfile.nc(token lost)file.nc?token=abc✅https://host/file.ncfile.ncfile.nc(unchanged)For HTTP-backed stores this is strictly better: query strings (presigned URLs, SAS tokens) were previously discarded and are now preserved.
Test
Adds a parametrized regression test covering
#,?, and;in keys. Passes with the fix, fails without it.pytest tests/test_registry.py -k "not async"is green (the 3 async-context-manager tests fail onmaintoo, unrelated to this change). Pre-commit (ruff/mypy/codespell) passes.Note for reviewers
If a presigned URL resolves to an already-credentialed object store (rather than a generic HTTP store), the reattached query becomes part of the key — a contradictory configuration, and out of scope for both motivations. Happy to gate the reconstruction on store type if you'd prefer.